Skip to content

feat(parameters): parameter conversion - #688

Open
azerupi wants to merge 3 commits into
azerupi/params/rust-rangesfrom
azerupi/params/parameter-conversion
Open

feat(parameters): parameter conversion#688
azerupi wants to merge 3 commits into
azerupi/params/rust-rangesfrom
azerupi/params/parameter-conversion

Conversation

@azerupi

@azerupi azerupi commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

The code changes in this PR have been assisted by Claude Code but I have reviewed and iterated on the code.

Problem

A parameter's conversion was defined only through trait resolution. ParameterVariant supplied the kind and conversion in both directions, and every read, write and declaration went through the type's conversion.

But this has 2 problems:

  1. The orphan rule makes it hard to use types from third-party crates as parameters because the trait ParameterVariant lives in rclrs and only rclrs can implement it for foreign types. This means that any type from the std or other crates that doesn't have a ParameterVariant trait implementation in rclrs can't be used by the user.
  2. Some types can have multiple ways to convert them. For example std::time::Duration could be stored in the parameter as secodns (f64) or milliseconds (i64) or even another representation. But you can only have one implementation of ParameterVariant.

Both of those cases required the user to make newtypes and implement ParameterVariant for them.

Solution

The conversion becomes a value that a declaration carries, rather than a trait implementation the type must have.

let seconds = ParameterConversion::double(
    Duration::as_secs_f64,
    Duration::try_from_secs_f64,
);

let timeout = node.declare_parameter_with("timeout", seconds)
    .default(Duration::from_millis(500))
    .mandatory()?;

let d: Duration = timeout.get();    // the handle is in the field's own type

Nothing about the existing API changes. declare_parameter::<T>(name) is now declare_parameter_with(name, ParameterConversion::of_variant::<T>()), and a type with a ParameterVariant impl declares exactly as before.

get_with and set_with give use_undeclared_parameters() the same reach, so a parameter whose type has no ParameterVariant is readable and writable there as well:

let undeclared = node.use_undeclared_parameters();
undeclared.set_with("timeout", Duration::from_secs(3), &seconds)?;
assert_eq!(undeclared.get_with("timeout", &seconds), Some(Duration::from_secs(3)));

Bug fix

ParameterKind does not identify a Rust type. Several types share one, and a value of the right kind can still be unrepresentable in the declared type, for example 70000 is a perfectly good Integer but not a u16.

In current main the parameter write paths only compared kinds, while every getter assumes the stored value converts to the Rust type. This means that setting a bad parameter could succeed and make the node panic. It is even worse when you think a parameter can be changed externally to the process.

So right now in main, a remote SetParameters call could store a value that passed validation and then panicked the node on the next read. And while this can't happen if we use the standard ROS types as parameters it is reachable today by anyone implementing the public ParameterVariant trait for a type whose TryFrom is partial. And this bug becomes unavoidable once narrow integers are used as parameters, which is a feature I want to add in a later PR.

The declaration's conversion is the check that needs to pass, so in the setters we call this function in addition to the kind check:

fn type_check_of<T: 'static>(conversion: ParameterConversion<T>) -> ValidateCallback {
    Arc::new(move |value| conversion.from_value(value.clone()).map(|_| ()))
}

ParameterValueError::Invalid carries the reason the conversion gave, and the parameter service puts it in the response, so a user running ros2 param set sees why a value was refused.

ParameterOptions was generic over the parameter type so that it could
hold that type's own Range, and a private ParameterOptionsStorage existed
alongside it to hold the erased ParameterRanges that the descriptor and
every range check actually use. The two structs differed in that one
field and nothing else, and each declaration path converted between them.

A range describes the values a parameter may hold, which is a property of
how the value is represented rather than of the Rust type it is read back
as. Holding the erased form throughout collapses the two structs into
one, and the three declaration paths stop converting at all.

range() still takes the parameter type's own Range, so the bounds are
still written in the units the field is read back in and a literal that
does not fit is still a compile error. Only where the value is stored
changes.

ParameterOptions also stops being public, which it only ever was by
accident. Its fields are private, no function takes or returns one, and
its only uses are as a private field of the builder and of the stored
parameter, so all a caller could do with the name was construct an empty
one through Default and then find nothing that would accept it.

Assisted-by: Claude:claude-opus-5 [Claude Code]
A parameter's conversion was reachable only through trait resolution.
ParameterVariant supplied the kind and both directions, and every read,
write and declaration went through the type. That shut out any type whose
crate does not own the trait: std::time::Duration cannot implement
ParameterVariant, and neither can a chrono or uom type, so configuring
one meant a newtype in the caller's crate that existed for no other
reason.

Move the conversion into a value. ParameterConversion holds the kind and
the two directions, a declaration fixes one, and every handle it produces
carries it. ParameterConversion::of_variant assembles one from what a
ParameterVariant already states, so a type that has an implementation
needs nothing further and declares exactly as before. The trait keeps
stating its kind and its conversions and does not also hand out an
assembled conversion, so there is no second way to say the same thing and
no way for the two to disagree. The orphan rules do not apply to values,
so declare_parameter_with takes a foreign type directly.

The conversion back from a parameter value is fallible, because that
value arrives from a parameter file or from a remote SetParameters call
and cannot be assumed representable. The constructors take the shapes
std's own functions already have, so a duration in seconds is
ParameterConversion::double(Duration::as_secs_f64,
Duration::try_from_secs_f64) with nothing written by hand.

get_with and set_with give the undeclared parameter API the same reach,
so a parameter whose type has no implementation is readable and writable
there too.

The handles' Debug implementations lose their ParameterVariant bound,
which was only ever needed for the conversion they now hold.

The range setter splits in two. range() keeps resolving to the parameter
type's own Range and moves to an impl block that still requires
ParameterVariant, so bounds stay in the units the parameter is read back
in and a literal that does not fit is still a compile error.
stored_ranges() takes the erased form, in the terms the conversion stores
the value in, for a parameter whose type has no Range to name. Two names
because the blocks overlap and one name would be a duplicate definition.

No existing test changed, which is what says the default conversion is
the one the types already described for themselves.

Assisted-by: Claude:claude-opus-5 [Claude Code]
A ParameterKind does not uniquely identify a Rust type. Several types can
share one, and a value of the right kind can still be unrepresentable in
the type a parameter was declared with. validate_parameter_setting only
compared kind discriminants and Parameters::set only compared kinds,
while every getter assumes the stored value converts back:

    self.conversion.from_value(..).ok().unwrap()

So a remote SetParameters call could store a value that passes validation
and then panic the node on the next read. This is reachable today by
anyone implementing the public ParameterVariant trait for a type whose
conversion from ParameterValue is partial, and becomes unavoidable once
narrow integer and enum parameter types exist.

Run the declaration's conversion on both write paths, before the range
check and before the custom validate callback, whose type-erasure wrapper
depends on the value being convertible. The check is the conversion the
declaration already carries, so there is nothing extra for a type to
implement and no way for it to accept a value that a later read would
then reject. ParameterValueError::Invalid carries the reason the
conversion gave back to the caller, which the parameter service puts in
the response so the operator who made the call can see why the value was
refused.

Assisted-by: Claude:claude-opus-5 [Claude Code]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant